Refactor broadcasting for a needs of 4844 #5485

marcindsobczak · 2023-03-22T18:05:34Z

Changes

added parameterless GetLength method to TransactionExtensions in TxPool project
Thanks to that, there is no more need to pass TxDecoder as an argument in Transaction.GetLength(decoder) (Core project doesn't have Rlp dependency).
added MaxSizeOfTxForBroadcast = 128KB
added method CanBeBroadcast in TransactionExtensions which is checking if tx is not blob-type and not larger than MaxSizeOfTxForBroadcast
added logic in Eth68ProtocolHandler which is dividing PersistentTxsToSend to full transactions (which CanBeBroadcast) and to tx hashes (which !CanBeBroadcast, so would be only announced)
added logic for sending recently added local tx - if is blob-type or too large, would be only announced
added additional protection from sending blob-type txs in SyncPeerProtocolHandlerBase
added plenty of tests

Types of changes

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

skip blob txs when preparing msg with persistent txs add test for skipping blob txs when picking persistent txs to broadcast add tests for broadcasting local blob txs just after receiving it one more place to protect fix whitespaces connected my changes one more whitespace

marcindsobczak · 2023-03-22T18:06:03Z

There is a regression in hive eth protocol test (TestTransaction), need to debug it

marcindsobczak · 2023-03-23T11:20:42Z

hive eth protocol 🟢 https://github.com/NethermindEth/nethermind/actions/runs/4498920158/jobs/7916126549 (no regression, same fail as on master)
hive engine 🟢 https://github.com/NethermindEth/nethermind/actions/runs/4499897225/jobs/7918337846 (no regression, same fail as on master)
posdao pre-merge 🟢 https://github.com/NethermindEth/nethermind/actions/runs/4498925447/jobs/7916138476
posdao post-merge 🟢 https://github.com/NethermindEth/nethermind/actions/runs/4498923786/jobs/7916134678

…for_4844

MarekM25 · 2023-04-06T13:45:50Z

src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/SyncPeerProtocolHandlerBase.cs

@@ -183,7 +182,18 @@ public virtual Task<byte[][]> GetNodeData(IReadOnlyList<Keccak> hashes, Cancella

        public void SendNewTransaction(Transaction tx)
        {
-            SendMessage(new[] { tx });
+            if (tx.Hash != null && NotifiedTransactions.Set(tx.Hash))


QQ: we haven't had this if tx.Hash != null && NotifiedTransactions.Set(tx.Hash) before. Is it fixing some bug, but not related to 4844?

It is recently added feature #5449
It was changed for SendNewTransactions:
https://github.com/NethermindEth/nethermind/pull/5449/files#diff-e2801b05e4ee2fda5592da789a82f0e4c733c8b20c8688020cbb0de71e3db5e5
and SendNewTransaction was skipped. I just added it here too.
It is a protection from sending the same tx few times to one peer - we are caching sent transactions. Not directly related to 4844

Would move this to ShouldNotifyTransaction method for this condition in both places.

deffrian · 2023-04-24T10:47:11Z

src/Nethermind/Nethermind.TxPool/TransactionExtensions.cs

+
+        public static int GetLength(this Transaction tx)
+        {
+            return tx.GetLength(_transactionSizeCalculator);


Maybe we can remove GetLength from transaction class and leave it as extension? And probably remove ITransactionSizeCalculator interface?

That was my idea as well, but was not working - look at tests after this commit: 9dbf58a
And it was working again after this one: 1ddfd28

Why it wasn't working? This sounds like a good idea.

Extension methods are static. In Transaction we have _size which is initialized as null and updated after first call of tx.GetLength(). If I move everything to extensions, all fields must be static. And _size can't be static.
I can add public Size in Transaction and update it in extension method, but then it will be exposed and can be easily used wrong way (before updating to actual tx length value) - it doesn't seem as a good idea.
Potentially I can drop size and then it can be fully in extensions, but length would be calculated after every call of tx.GetLength(), not only in first one. That doesn't seem as a good idea as well

LukaszRozmej · 2023-04-26T08:57:20Z

src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/SyncPeerProtocolHandlerBase.cs

@@ -183,7 +182,18 @@ public virtual Task<byte[][]> GetNodeData(IReadOnlyList<Keccak> hashes, Cancella

        public void SendNewTransaction(Transaction tx)
        {
-            SendMessage(new[] { tx });
+            if (tx.Hash != null && NotifiedTransactions.Set(tx.Hash))


Would move this to ShouldNotifyTransaction method for this condition in both places.

LukaszRozmej · 2023-04-26T08:57:55Z

src/Nethermind/Nethermind.Network/P2P/ProtocolHandlers/SyncPeerProtocolHandlerBase.cs

@@ -218,7 +228,7 @@ protected virtual void SendNewTransactionsCore(IEnumerable<Transaction> txs, boo
                    packetSizeLeft = TransactionsMessage.MaxPacketSize;
                }

-                if (tx.Hash is not null)
+                if (tx.Hash is not null && tx.Type != TxType.Blob) //additional protection from sending full blob-type txs


Isn't there tx.SupportsBlobs that would be better for future as an abstraction than just checking Type?

LukaszRozmej · 2023-04-26T08:59:47Z

src/Nethermind/Nethermind.TxPool/TransactionExtensions.cs

    {
-        public static UInt256 CalculateGasPrice(this Transaction tx, bool eip1559Enabled, in UInt256 baseFee)
+        private static readonly int MaxSizeOfTxForBroadcast = 128_000; //128KB, as proposed in https://eips.ethereum.org/EIPS/eip-5793


wonder if they meant 128000 or 131072 bytes (1024*128)

Actually spec is talking about KB and KB = 1024B. Good catch. It's only proposed value and not strongly imposed, but I will change it - we will be more accurate

LukaszRozmej · 2023-04-26T09:01:25Z

src/Nethermind/Nethermind.TxPool/TransactionExtensions.cs

+
+        public static int GetLength(this Transaction tx)
+        {
+            return tx.GetLength(_transactionSizeCalculator);


Why it wasn't working? This sounds like a good idea.

LukaszRozmej · 2023-04-26T09:02:43Z

src/Nethermind/Nethermind.TxPool/TransactionExtensions.cs

+            return tx.GetLength(_transactionSizeCalculator);
+        }
+
+        public static bool CanBeBroadcast(this Transaction tx) => tx.Type != TxType.Blob && tx.GetLength() <= MaxSizeOfTxForBroadcast;


tx.SupportsBlobs

LukaszRozmej · 2023-04-26T09:04:25Z

src/Nethermind/Nethermind.TxPool/TxBroadcaster.cs

+            List<Transaction>? persistentTxsToSend = null;
+            List<Transaction>? persistentHashesToSend = null;


Can we use ArrayPoolList's? I think we could.

Then we would need to declare it one method higher (in BroadcastPersistentTxs()) and pass both to GetPersistentTxsToSend() - otherwise it will be disposed too quick. We are calculating numberOfPersistentTxsToBroadcast (so max size of collections) in GetPersistentTxsToSend() - we would need to move it higher as well. It would be a bit ugly. And taking into consideration that

this method is called once per block

if there are no persistent txs (as in most of our nodes) right now we are not allocating anything - lists are nulls until there is an item eligible to add

using ArrayPoolList would be called no matter if we have eligible items

Actually it would be good to add basic check _persistentTxs.Count == 0 in the beginning and quick return if collection is empty. And then using ArrayPoolList would have more sense. But then most of the logic from GetPersistentTxsToSend() would be moved higher, to BroadcastPersistentTxs(), so we could move everything and drop GetPersistentTxsToSend() at all. And we will still need to do using ArrayPoolList for both collections even if one would not have any items

I will add quick return in case of empty collection - it's a good idea. And about ArrayPoolList's - if you want I can do further refactor, but I'm not convinced to that idea

Hm... Then instead of using ArrayPoolList, maybe we can have a List<> instance we can reuse? As this is single-threaded if I understand it correctly.

marcindsobczak added 5 commits March 21, 2023 14:43

move tx size logic to extension

2f9edf6

adjust broadcaster

744e9d7

adjust handlers

3b23142

remove redundant check

9dbf58a

marcindsobczak added 2 commits March 22, 2023 20:32

fix?

1ddfd28

add test for MaxSizeOfTxForBroadcast

8a35cf5

marcindsobczak added 3 commits March 23, 2023 12:21

cosmetic

d15e6d4

cosmetic

00674e7

Merge remote-tracking branch 'origin/master' into feature/networking_…

761a51e

…for_4844

marcindsobczak marked this pull request as ready for review March 23, 2023 12:38

marcindsobczak requested review from LukaszRozmej, deffrian and flcl42 March 23, 2023 12:38

marcindsobczak added network eip-4844 labels Mar 23, 2023

marcindsobczak linked an issue Mar 23, 2023 that may be closed by this pull request

Refactor TxBroadcaster to not broadcast full blob txs #4968

Closed

marcindsobczak requested review from benaadams and MarekM25 March 28, 2023 08:28

MarekM25 approved these changes Apr 6, 2023

View reviewed changes

flcl42 mentioned this pull request Apr 19, 2023

EIP-4844: Shard Blob Transactions #4558

Closed

deffrian reviewed Apr 24, 2023

View reviewed changes

marcindsobczak merged commit 54df8aa into master Apr 25, 2023

marcindsobczak deleted the feature/networking_for_4844 branch April 25, 2023 08:26

LukaszRozmej reviewed Apr 26, 2023

View reviewed changes

marcindsobczak mentioned this pull request Apr 26, 2023

Minor fixes/improvements to tx broadcasting #5619

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor broadcasting for a needs of 4844 #5485

Refactor broadcasting for a needs of 4844 #5485

marcindsobczak commented Mar 22, 2023 •

edited

Loading

marcindsobczak commented Mar 22, 2023

marcindsobczak commented Mar 23, 2023 •

edited

Loading

MarekM25 Apr 6, 2023

marcindsobczak Apr 6, 2023

LukaszRozmej Apr 26, 2023

deffrian Apr 24, 2023

marcindsobczak Apr 24, 2023

LukaszRozmej Apr 26, 2023

marcindsobczak Apr 26, 2023

LukaszRozmej Apr 26, 2023

LukaszRozmej Apr 26, 2023

LukaszRozmej Apr 26, 2023

marcindsobczak Apr 26, 2023

LukaszRozmej Apr 26, 2023

LukaszRozmej Apr 26, 2023

LukaszRozmej Apr 26, 2023

marcindsobczak Apr 26, 2023

LukaszRozmej Apr 26, 2023

		List<Transaction>? persistentTxsToSend = null;
		List<Transaction>? persistentHashesToSend = null;

Refactor broadcasting for a needs of 4844 #5485

Refactor broadcasting for a needs of 4844 #5485

Conversation

marcindsobczak commented Mar 22, 2023 • edited Loading

Changes

Types of changes

Testing

Requires testing

If yes, did you write tests?

marcindsobczak commented Mar 22, 2023

marcindsobczak commented Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcindsobczak commented Mar 22, 2023 •

edited

Loading

marcindsobczak commented Mar 23, 2023 •

edited

Loading